-
Notifications
You must be signed in to change notification settings - Fork 366
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow deleting custom lists #5820
Conversation
816c571
to
29f802e
Compare
6c0930e
to
0fa069d
Compare
0141ab3
to
4d11f05
Compare
0fa069d
to
5d44827
Compare
4d11f05
to
13dbec5
Compare
5d44827
to
bf63d53
Compare
1da87ee
to
cd07be0
Compare
cd07be0
to
ed1b81e
Compare
6e77dc7
to
1189e5f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving because of no blocking changes
Reviewed 1 of 16 files at r2, 17 of 17 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @rablador)
ios/MullvadVPN/Coordinators/CustomLists/CustomListViewController.swift
line 178 at r3 (raw file):
title: NSLocalizedString( "CUSTOM_LISTS_DELETE_BUTTON", tableName: "APIAccess",
Shouldn't those table names be different ? (i.e. custom list table names ?)
This message is valid for all localised strings in custom lists
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @buggmagnet)
ios/MullvadVPN/Coordinators/CustomLists/CustomListViewController.swift
line 178 at r3 (raw file):
Previously, buggmagnet wrote…
Shouldn't those table names be different ? (i.e. custom list table names ?)
This message is valid for all localised strings in custom lists
Yeah, my bad, I'll change it.
43a5af2
to
e3af6b9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 15 of 18 files reviewed, 4 unresolved discussions (waiting on @buggmagnet and @rablador)
ios/MullvadVPN/Coordinators/CustomLists/CustomListViewController.swift
line 161 at r4 (raw file):
private func onDelete() { showDeleteAlert()
FMPOV I don't see any point of making function for showDeleteAlert
. the body of showDeleteAlert
can be put in the onDelete
directly
ios/MullvadVPN/Coordinators/CustomLists/CustomListViewController.swift
line 171 at r4 (raw file):
"CUSTOM_LISTS_DELETE_PROMPT", tableName: "CustomLists", value: "Delete \(subject.value.name)?",
based on the Figma's design it should be Delete custom list “the name of custom list”?
ios/MullvadVPN/Coordinators/CustomLists/EditCustomListCoordinator.swift
line 28 at r4 (raw file):
navigationController: UINavigationController, customListInteractor: CustomListInteractorProtocol, customList: CustomList
I think it should be part of interactor
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 15 of 18 files reviewed, 4 unresolved discussions (waiting on @buggmagnet and @mojganii)
ios/MullvadVPN/Coordinators/CustomLists/CustomListViewController.swift
line 161 at r4 (raw file):
Previously, mojganii wrote…
FMPOV I don't see any point of making function for
showDeleteAlert
. the body ofshowDeleteAlert
can be put in theonDelete
directly
Done.
ios/MullvadVPN/Coordinators/CustomLists/CustomListViewController.swift
line 171 at r4 (raw file):
Previously, mojganii wrote…
based on the Figma's design it should be
Delete custom list “the name of custom list”?
Yeah, but api access methods has a similar view that only says "Delete ". I thought it should be consistent, but perhaps it's better to follow the designs.
ios/MullvadVPN/Coordinators/CustomLists/EditCustomListCoordinator.swift
line 28 at r4 (raw file):
Previously, mojganii wrote…
I think it should be part of
interactor
.
I thought about it, but the custom list supplied to EditCustomListCoordinator
is specific to that coordinator only and has no relation to CustomListInteractorProtocol
or views where CustomListInteractorProtocal
is used.
4620229
to
627a022
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r5.
Reviewable status: 16 of 18 files reviewed, 3 unresolved discussions (waiting on @mojganii)
ios/MullvadVPN/Coordinators/CustomLists/CustomListViewController.swift
line 171 at r4 (raw file):
Previously, rablador (Jon Petersson) wrote…
Yeah, but api access methods has a similar view that only says "Delete ". I thought it should be consistent, but perhaps it's better to follow the designs.
I just talked to the design team about this, they said "we should follow what the desktop does"
Here included is a screenshot of the desktop.
We can follow the copy at least, if the bold title is too complicated to make, we can skip it for now
627a022
to
55f1db4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 14 of 19 files reviewed, 3 unresolved discussions (waiting on @buggmagnet and @mojganii)
ios/MullvadVPN/Coordinators/CustomLists/CustomListViewController.swift
line 171 at r4 (raw file):
Previously, buggmagnet wrote…
I just talked to the design team about this, they said "we should follow what the desktop does"
Here included is a screenshot of the desktop.We can follow the copy at least, if the bold title is too complicated to make, we can skip it for now
Done.
55f1db4
to
1f599ce
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 3 files at r4, 3 of 3 files at r6, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @mojganii)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! all files reviewed, all discussions resolved
ios/MullvadVPN/Coordinators/CustomLists/EditCustomListCoordinator.swift
line 28 at r4 (raw file):
Previously, rablador (Jon Petersson) wrote…
I thought about it, but the custom list supplied to
EditCustomListCoordinator
is specific to that coordinator only and has no relation toCustomListInteractorProtocol
or views whereCustomListInteractorProtocal
is used.
nit: maybe I need to change the place of the dependency.for now let's leave it
1f599ce
to
eb9edc7
Compare
🚨 End to end tests failed. Please check the failed workflow run. |
It should be possible to delete a custom list.
This change is